Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Made ap-change be calculated without LP. #4250

Merged
merged 1 commit into from
Nov 7, 2023

Conversation

orizi
Copy link
Collaborator

@orizi orizi commented Oct 12, 2023

Stack:

⚠️ Part of a stack created by spr. Do not merge manually using the UI - doing so may have unexpected results.


This change is Reviewable

@orizi orizi force-pushed the pr/orizi/ap-change-solve/eb28e7c1 branch from 49c49a3 to d8357e1 Compare October 12, 2023 10:10
@orizi orizi force-pushed the pr/orizi/ap-change-solve/b1c84e1f branch from 1c972c1 to 52dfa6b Compare October 12, 2023 10:10
@orizi orizi force-pushed the pr/orizi/ap-change-solve/eb28e7c1 branch from d8357e1 to adf0d6b Compare October 15, 2023 17:26
@orizi orizi force-pushed the pr/orizi/ap-change-solve/b1c84e1f branch from 52dfa6b to 81916e1 Compare October 15, 2023 17:26
Copy link
Contributor

@gilbens-starkware gilbens-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 5 files at r1, all commit messages.
Reviewable status: 2 of 5 files reviewed, 2 unresolved discussions (waiting on @liorgold2 and @orizi)


crates/cairo-lang-sierra-ap-change/src/compute.rs line 54 at r1 (raw file):

}

/// Helper to implement the `InvocationApChangeInfoProvider` for the equation generation.

Wrong doc.


crates/cairo-lang-sierra-ap-change/src/compute.rs line 134 at r1 (raw file):

                | ApChange::Known(_)
                | ApChange::DisableApTracking => {
                    if let Some(locals) = self.locals.get(&idx) {

Consider changing it to locals_count.

Code quote:

locals

Copy link
Collaborator Author

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 2 of 5 files reviewed, 2 unresolved discussions (waiting on @gilbens-starkware and @liorgold2)


crates/cairo-lang-sierra-ap-change/src/compute.rs line 54 at r1 (raw file):

Previously, gilbens-starkware (Gil Ben-Shachar) wrote…

Wrong doc.

Done.


crates/cairo-lang-sierra-ap-change/src/compute.rs line 134 at r1 (raw file):

Previously, gilbens-starkware (Gil Ben-Shachar) wrote…

Consider changing it to locals_count.

it isn't count - it is size. Done.

@orizi orizi changed the base branch from pr/orizi/ap-change-solve/eb28e7c1 to main October 19, 2023 03:00
@orizi orizi force-pushed the pr/orizi/ap-change-solve/b1c84e1f branch from 81916e1 to cf9605b Compare October 19, 2023 03:00
Copy link
Contributor

@gilbens-starkware gilbens-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 2 of 5 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @liorgold2 and @orizi)


crates/cairo-lang-sierra-ap-change/src/compute.rs line 319 at r2 (raw file):

    }

    /// Returns the locals count for a statement.

Suggestion:

e locals size

crates/cairo-lang-sierra-ap-change/src/compute.rs line 354 at r2 (raw file):

}

/// Calculates gas information for a given program.

Suggestion:

/// Calculates the ap change information for a given program.

crates/cairo-lang-sierra-ap-change/src/lib.rs line 54 at r2 (raw file):

}

/// Calculates gas information for a given program.

Suggestion:

ap change

@orizi orizi force-pushed the pr/orizi/ap-change-solve/b1c84e1f branch from cf9605b to 76c4d5e Compare October 19, 2023 12:05
Copy link
Contributor

@gilbens-starkware gilbens-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @liorgold2)

@orizi orizi changed the base branch from main to sierra-minor-update November 7, 2023 08:43
@orizi orizi force-pushed the pr/orizi/ap-change-solve/b1c84e1f branch from 76c4d5e to 02eabd7 Compare November 7, 2023 08:44
Copy link
Contributor

@gilbens-starkware gilbens-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @liorgold2)

@orizi orizi force-pushed the pr/orizi/ap-change-solve/b1c84e1f branch from 02eabd7 to 7739d89 Compare November 7, 2023 14:12
@orizi orizi changed the base branch from sierra-minor-update to pr/orizi/ap-change-solve/81e4ca83 November 7, 2023 14:12
Copy link
Contributor

@gilbens-starkware gilbens-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 9 of 9 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @liorgold2)

@orizi orizi changed the base branch from pr/orizi/ap-change-solve/81e4ca83 to main November 7, 2023 15:27
@orizi orizi force-pushed the pr/orizi/ap-change-solve/b1c84e1f branch from 7739d89 to 0cb35de Compare November 7, 2023 15:28
@orizi orizi changed the base branch from main to pr/orizi/ap-change-solve/81e4ca83 November 7, 2023 15:28
Copy link
Contributor

@gilbens-starkware gilbens-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 5 of 5 files at r6, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @liorgold2)

@orizi orizi force-pushed the pr/orizi/ap-change-solve/b1c84e1f branch from 0cb35de to bbb47e2 Compare November 7, 2023 15:54
@orizi orizi force-pushed the pr/orizi/ap-change-solve/81e4ca83 branch from bd2fb94 to 02ee2cd Compare November 7, 2023 15:54
Copy link
Contributor

@gilbens-starkware gilbens-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @liorgold2)

@orizi orizi force-pushed the pr/orizi/ap-change-solve/b1c84e1f branch from bbb47e2 to 6835021 Compare November 7, 2023 16:02
@orizi orizi force-pushed the pr/orizi/ap-change-solve/81e4ca83 branch from 02ee2cd to 5a35253 Compare November 7, 2023 16:02
@orizi orizi changed the base branch from pr/orizi/ap-change-solve/81e4ca83 to main November 7, 2023 16:07
@orizi orizi force-pushed the pr/orizi/ap-change-solve/b1c84e1f branch from 6835021 to 7079d03 Compare November 7, 2023 16:07
@orizi orizi changed the base branch from main to pr/orizi/ap-change-solve/81e4ca83 November 7, 2023 16:07
@orizi orizi changed the base branch from pr/orizi/ap-change-solve/81e4ca83 to main November 7, 2023 16:20
@orizi orizi force-pushed the pr/orizi/ap-change-solve/b1c84e1f branch from 7079d03 to 5ca52af Compare November 7, 2023 16:20
@orizi orizi enabled auto-merge November 7, 2023 16:20
@orizi orizi added this pull request to the merge queue Nov 7, 2023
Merged via the queue into main with commit 7df7978 Nov 7, 2023
38 checks passed
@orizi orizi deleted the pr/orizi/ap-change-solve/b1c84e1f branch November 8, 2023 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants